-
-
Notifications
You must be signed in to change notification settings - Fork 348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
syntax: simplify unset-or-null with null default to just unset #849
Conversation
If the default for an unset-or-null parameter expansion is null, the or-null part of the expansion is redundant and can be removed. The result is null on match either way.
2755b94
to
5635541
Compare
Docker build failure/timeout appears unrelated. |
I'm slightly uneasy about simplifications like these. Are you absolutely sure that this can't break user programs? I wonder if turning an unset to null can matter, for example. |
I cannot think of a case that this could break. There certainly is a difference between unset and null valued variables (for example |
Alright, SGTM. We can always revert if our thinking here is wrong. |
Example failed workflow: https://github.com/dfinity/sdk/actions/runs/3697173489/jobs/6262516972 This is the change to shfmt 3.6.0: mvdan/sh#849 - Simplify ${name:-} to the equivalent ${name-} - #849
Example failed workflow: https://github.com/dfinity/sdk/actions/runs/3697173489/jobs/6262516972 This is the change introduced in shfmt 3.6.0: mvdan/sh#849 - `Simplify ${name:-} to the equivalent ${name-}` - [#849](mvdan/sh#849)
ref: mvdan/sh#849 Signed-off-by: Abirdcfly <[email protected]>
ref: mvdan/sh#849 Signed-off-by: Abirdcfly <[email protected]>
If the default for an unset-or-null parameter expansion is null, the or-null part of the expansion is redundant and can be removed. The result is null on match either way.